Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Change some WiFi buffer settings to match esp-idf. See #2899 #2912

Merged
merged 1 commit into from
Jul 9, 2019
Merged

Change some WiFi buffer settings to match esp-idf. See #2899 #2912

merged 1 commit into from
Jul 9, 2019

Conversation

lbernstone
Copy link
Contributor

Buffer mismatches appear to be causing WiFi communications to halt. See #2899 for more details.

@stickbreaker
Copy link
Contributor

@lbernstone since you found these relationships between buffer configurations, why don't you add some error checking for the values. added these compiler checks would prohibit this error from happening again.

This is just an example, You will have to figure out the correct rules to test.

#if (CONFIG_ESP32_WIFI_RX_BA_WIN > CONFIG_ESP32_WIFI_STATIC_RX_BUFFER_NUM )
# error buffer config mismatch
#endif 

Do you know the limit values? Are the current values in your PR "empirical" or calculated?

Chuck.

@lbernstone
Copy link
Contributor Author

They are taken from the default sdkconfig in esp-idf, so not really even empirically derived. They are compiled into the libraries, so I'm not sure what an error check would do for anybody but travis. The error check really ought to be upstream in LWiP, where the mismatch is actually causing the problem.

@atanisoft
Copy link
Collaborator

They are compiled into the libraries, so I'm not sure what an error check would do for anybody but travis.

These are in fact compiled at the time of compiling arduino-esp32 code and not IDF. This is not always the case though. The WiFi library itself uses these values when it initializes the wifi stack.

@r1dd1ck
Copy link

r1dd1ck commented Jun 21, 2019

These checks will be implemented @ ESP-IDF sooner or later, so I would stay with just fixing the values in the current sdkconfig now & let the checks come in later with an IDF update.

@lbernstone
Copy link
Contributor Author

@atanisoft

These are in fact compiled at the time of compiling arduino-esp32 code and not IDF. This is not always the case though. The WiFi library itself uses these values when it initializes the wifi stack.

Doh, you are right. Only used in headers.

@atanisoft
Copy link
Collaborator

Doh, you are right. Only used in headers.

@r1dd1ck is the one he who pointed this out. Almost all settings are precompiled in but there seem to be a handful that are adjustable at the app level.

It would be really nice to be able to override these defaults in our app code without modifying core headers but that isn't possible or maintainable.

@lbernstone
Copy link
Contributor Author

@atanisoft : Copy the header into your sketch folder. Arduino will use the local copy instead of the include folder version.

@atanisoft
Copy link
Collaborator

@lbernstone now that is an awesome find!

@Daemach
Copy link

Daemach commented Jun 22, 2019 via email

@atanisoft
Copy link
Collaborator

@Daemach sdkconfig.h I believe

@stickbreaker
Copy link
Contributor

@Daemach tools/sdk/include/config/sdkconfig.h The other is file is used for the build config.

Chuck.

@me-no-dev
Copy link
Member

how does the heap look when you increase the static buffers?

@r1dd1ck
Copy link

r1dd1ck commented Jun 26, 2019

@me-no-dev
If I remember correctly, it takes about 3-4kB more than with the current arduino defaults (8).
Raising it to the iperf example defaults (16) had a 13-14kB impact.

Maybe it could survive even without the static buffer raise, as I suppose the most crucial part was the RX_BA_WIN adjustment (16 → 6). Haven't played around with the numbers since then, but the values from this PR have proven to be quite solid, and they are also the defaults for ESP-IDF.

@lbernstone
Copy link
Contributor Author

Those static buffers should be 1K each. In practice, I see a difference of 3120 bytes between the two configs.

@me-no-dev
Copy link
Member

alright... I'm merging this to include it in the new build

@me-no-dev me-no-dev merged commit b0d8d4d into espressif:master Jul 9, 2019
@lbernstone lbernstone deleted the WIFI_RX_BA_WIN branch August 27, 2019 18:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants